Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update CI image and add mkmf for fre make testing #245

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

update CI image and add mkmf for fre make testing #245

wants to merge 4 commits into from

Conversation

rem1776
Copy link
Contributor

@rem1776 rem1776 commented Nov 1, 2024 •
edited
Loading

Describe your changes

This PR updates the existing CI workflow that runs pytest to support compilation of models using fre make, as well as container creation.

  • Changes the CI image to a newly created one with gcc, mpich and anything else needed to test fre make capabilities more thoroughly. This replaces the 'stock' image we were using from github and has miniconda 24.7.1 and gcc 14
  • Adds mkmf as a git submodule
  • Adds a new test file that checks the run-fremake subcommand compiles the null model successfully using an added CI platform
  • Made changes to the workflow to run on the new image and cleaned up a few commands that don't seem necessary

Leaving this as a draft for now since its running into a callback error during run-fremake.
The test still needs some updates to call the routine differently.

Issue ticket number and link (if applicable)

#226
Fixes #229

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback

@ilaflott
Copy link
Member

ilaflott commented Nov 1, 2024 •
edited
Loading

I'm not reviewing this yet, but i did want to bring your attention to my attempt to package fms_yaml_tools with gfdl_msd_schemas as a git submodule. If i'm not overlooking something, your approach will run into this

TBH, i don't think i exhausted all possibilities/workarounds, but from my googling at the time, the situation doesn't look fun. I would like to see you succeed with this, because then making fms_yaml_tools a proper conda package for fre-cli will be MUCH easier. I.e. PLEASE prove me wrong about conda build / git submodule, i would really prefer to be wrong.

@rem1776
Copy link
Contributor Author

rem1776 commented Nov 4, 2024

I'm not reviewing this yet, but i did want to bring your attention to my attempt to package fms_yaml_tools with gfdl_msd_schemas as a git submodule. If i'm not overlooking something, your approach will run into this

TBH, i don't think i exhausted all possibilities/workarounds, but from my googling at the time, the situation doesn't look fun. I would like to see you succeed with this, because then making fms_yaml_tools a proper conda package for fre-cli will be MUCH easier. I.e. PLEASE prove me wrong about conda build / git submodule, i would really prefer to be wrong.

I haven't gotten that error in the CI testing or when I ran it in the container manually. Did you try starting off that CI with the recursive checkout option like i did for this one? I think pulling in the submodule after the conda env create might be part of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilaflott ilaflott Awaiting requested review from ilaflott

@singhd789 singhd789 Awaiting requested review from singhd789

@kiihne-noaa kiihne-noaa Awaiting requested review from kiihne-noaa

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mkmf not included in installation
2 participants
@rem1776 @ilaflott